feat(engine): add rule evaluation coverage contract (#263) - #321
feat(engine): add rule evaluation coverage contract (#263)#321dipeshrayg wants to merge 2 commits into
Conversation
scan() only ever reports violations, so a scan with no findings for a rule is indistinguishable from compliant, not-applicable, and never evaluated. get_compliance_score() inferred PASS from the absence of a finding, silently treating errored or unmigrated rules as passing. Rules can now additionally expose evaluate(azure_client, subscription_id) -> List[RuleEvaluation], reporting a PASS/FAIL/ UNKNOWN/ERROR/NOT_APPLICABLE status per resource instead of only per violation. This is additive: scan() is unchanged, and a rule without evaluate() still runs, its coverage recorded as UNKNOWN/ LEGACY_RULE_NOT_MIGRATED rather than assumed to be a pass. - New rule_evaluations table (migration chained after openshield-org#308's head), with CHECK constraints on the five statuses, a non-empty canonical resource_id, and a required reason_code for UNKNOWN/ERROR/ NOT_APPLICABLE. FAIL evaluations get a nullable finding_id FK, backfilled in the same transaction as the finding insert. - Engine wiring: evaluator exceptions produce an ERROR at a canonical rule/subscription scope rather than vanishing; a FAIL evaluation contributes its own finding only when scan() hasn't already reported the same (rule_id, resource_id), so a rule implementing both never double-counts. - get_compliance_score() now derives PASS/FAIL/UNKNOWN/ERROR/ NOT_APPLICABLE from rule_evaluations instead of inferring PASS from missing findings, and returns evaluated/passed/failed/unknown/error/ not_applicable counts separately. UNKNOWN and ERROR never improve the score. - AZ-KV-006 migrated as the reference evaluate() implementation. Single Alembic head and migration-chain checks, plus populated-database CHECK constraint tests gated on DATABASE_URL (CI already runs these against a live migrated Postgres). Signed-off-by: Dipesh Ray <dipesh.ray.g@gmail.com>
- Join the two-line CHECK constraint SQL string in the rule_evaluations migration onto one line, under the 120-char limit. - Add the blank line ruff format wants before a top-level def in the evaluate() code sample, and drop an em dash from the surrounding text. Signed-off-by: Dipesh Ray <dipesh.ray.g@gmail.com>
|
@dipeshrayg, this remains the canonical review track for #263’s evaluation semantics: status vocabulary, per-resource evidence rows, reason-code constraints, finding linkage, aggregation, and the reference-rule migration. Please keep it draft while we align its migration and persistence API with #325. Do not widen it into leases, admission, enrichment, or finding lifecycle; those belong to separate layers. |
|
Understood, keeping this in draft and staying within the evaluation-semantics scope you outlined. No leases, admission, enrichment, or finding lifecycle here. I looked at #325 to see where the overlap is concretely: its I'll hold off on touching the migration or persistence code here until you've decided how the two should line up. Happy to adjust either the migration chain or the persistence approach once that's settled, whichever way you want it to land. |
|
Cross-PR coordination note: #325 (scan leases/fencing/idempotency, #303) also creates a rule_evaluations table and its own persistence semantics, which overlaps this PR. Per m-khan, this PR is the agreed #263 evaluation-contract implementation, so #325 should drop its copy and keep only the fencing/idempotency layer. Sharing the side-by-side here so the reconciliation is clear on both sides. Schema: rule_evaluations
The core columns, keys, and constraints overlap and share names, so whichever migration runs second fails with "relation already exists." This PR is the strict superset: it adds the rule_id and status indexes and the reason_code-required CHECK. Producer (who emits evaluations)
Only this PR makes evaluations observable; #325 has no producer, so its evaluations would always be empty in production. save_scan persistence semanticsBoth PRs are full rewrites of the same method with incompatible idempotency models, so the conflict is larger than the table.
Compliance score (the actual #263 bug)
Only this PR fixes the score-inflation bug. Recommended resolution
One integration detail for whoever reconciles: this PR's engine adds evaluate()-derived FAIL findings to the findings list, and #325 derives finding_key from rule_id plus resource scope plus discriminator. Those compose, but make sure evaluate()-derived findings get stable finding_keys so the upsert stays idempotent. |
|
Thanks for the detailed side by side, that matches what I found and goes further. Agreed with the recommended resolution: this PR owns the schema, scanner/evaluation.py, the engine producer, aggregation, and the compliance-score fix, #325 keeps leases/fencing/admission/enrichment/metrics and drops its rule_evaluations copy. On the save_scan reconciliation: agreed the real merge point is there, not just the table, and that the final version should be #325's fenced upsert skeleton with this PR's evaluation field set and FAIL-to-finding_id linkage folded in, using upsert-plus-delete-absent for evaluations to match #303's replay-safety model rather than this PR's current per-scan delete-and-reinsert. I'll hold off implementing that here until the ordering is confirmed, since it depends on #325's lease/fencing columns and finding_key concept that don't exist in this PR's schema. Also agreed on the finding_key point for evaluate()-derived findings, once that reconciliation happens I'll make sure AZ-KV-006's FAIL findings produce a stable, discriminator-safe key so the upsert in #325's model stays idempotent. Ready whenever the merge order is decided. |
3 participants
What does this PR do?
Adds the rule evaluation coverage contract from #263: rules can now report a status for every resource they looked at (PASS included), so a scan that never ran, errored, or came from an unmigrated rule can no longer be silently read as a pass.
Type of change
Testing
alembic headsresolves to exactly one head)Related issue
Addresses #263
What changed
scanner/evaluation.py:EvaluationStatus(PASS/FAIL/UNKNOWN/ERROR/NOT_APPLICABLE),RuleEvaluationdataclass,subscription_scope_id()for canonical non-empty scope identifiers, andaggregate_status()implementing the conservative FAIL > ERROR > UNKNOWN > PASS > NOT_APPLICABLE roll-up order.alembic/versions/3f59f83a5253_rule_evaluations.py, chained onto fix(core): enforce severity contract v1 #308's head (d8e4f6a1b2c3).rule_evaluationstable with CHECK constraints on the five statuses, a non-emptyresource_id, and a requiredreason_codefor UNKNOWN/ERROR/NOT_APPLICABLE.finding_idis a nullable FK, populated only for FAIL evaluations.scanner/engine.py: callsrule.evaluate()when a rule exposes it. A rule withoutevaluate()gets one UNKNOWN/LEGACY_RULE_NOT_MIGRATED row instead of silently having no coverage. Anevaluate()exception (or a non-list return) produces one ERROR at the canonical rule/subscription scope. A FAIL evaluation contributes its own finding only whenscan()hasn't already reported the same(rule_id, resource_id), so a rule implementing both never double-counts.api/models/finding.py:save_scan()persistsrule_evaluationsin the same transaction as findings and backfillsfinding_idfor FAIL rows via a join on(scan_id, rule_id, resource_id).get_compliance_score()now derives each control's status fromrule_evaluations(never from finding absence), returnsevaluated/passed/failed/unknown/error/not_applicablecounts separately, and excludes UNKNOWN/ERROR from the numerator so they can never improve the score. Response carriescontract_version: "2".scanner/rules/az_kv_006.py: migrated as the one reference rule.evaluate()reports PASS/FAIL per vault, UNKNOWN for a vault missing its properties payload (previously silently skipped byscan()), and NOT_APPLICABLE when the vault list is empty (sinceAzureClient.get_key_vaults()can't distinguish "no vaults" from "the list call failed"; closing that gap is the deferred ARG cross-check).docs/adding-a-rule.md: short opt-in section documenting theevaluate()contract for future rule migrations.Regression coverage
evaluate()) is recorded as UNKNOWN/LEGACY_RULE_NOT_MIGRATED, never PASS.evaluate()exception produces ERROR at canonical scope instead of vanishing.evaluate()returning a non-list is treated as an error, not silently accepted.scan()didn't already report it.scan()already reported.rule_evaluationsrows persist in the same transaction as findings, withfinding_idcorrectly linked (FAIL) or left null (UNKNOWN/ERROR/PASS/NOT_APPLICABLE).rule_evaluationsrows atomically, same as findings.aggregate_status()conservative ordering (FAIL beats ERROR beats UNKNOWN beats PASS beats NOT_APPLICABLE) under every input order.get_compliance_score(): a clean scan with real PASS evaluation rows shows PASS; a clean scan with zero evaluation rows shows UNKNOWN, not PASS (the bug this PR fixes); a remediated rule with a new PASS row shows PASS; worst-severity aggregation across findings is unchanged.evaluate(): compliant vault -> PASS, non-compliant -> FAIL with the finding embedded, missing properties -> UNKNOWN, empty vault list -> NOT_APPLICABLE, mixed vaults report one status each.resource_id, reject a NULLreason_codeon UNKNOWN, and accept a valid PASS row withfinding_idleft null.d8e4f6a1b2c3.Scope
Matches what was agreed on #263: the contract, persistence/aggregation, and one end-to-end reference rule (AZ-KV-006). The Azure Resource Graph completeness cross-check stays a separate follow-up, as discussed.
Checklist
Signed-off-bytrailer